Block Header should use waveEnv for context menu (for mocking)#3041
Block Header should use waveEnv for context menu (for mocking)#3041
Conversation
Deploying waveterm with
|
| Latest commit: |
7282e08
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6b574de7.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-blockheader-contextmen.waveterm.pages.dev |
WalkthroughThe changes refactor context menu handling across three files. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Changes ReviewedThe PR makes three related changes:
All changes appear correct and follow the project's existing patterns. The refactoring moves from a static singleton to dependency injection via WaveEnv, which improves testability. Files Reviewed (3 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/app/block/blockframe-header.tsx (1)
179-179: Minor naming inconsistency:waveEnvvsblockEnv.
BlockFrame_HeaderuseswaveEnv(line 179) whileHeaderEndIconsusesblockEnv(line 116) for the same typed value (BlockEnv). Both are correct, but consistent naming would improve readability.♻️ Optional: Rename for consistency
- const waveEnv = useWaveEnv<BlockEnv>(); + const blockEnv = useWaveEnv<BlockEnv>(); const metaView = jotai.useAtomValue(waveEnv.getBlockMetaKeyAtom(nodeModel.blockId, "view"));(Would require updating all
waveEnvreferences inBlockFrame_HeadertoblockEnv.)Also applies to: 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/blockframe-header.tsx` at line 179, The file has inconsistent variable naming: BlockFrame_Header uses waveEnv from useWaveEnv<BlockEnv>() while HeaderEndIcons and other places use blockEnv; rename the variable in BlockFrame_Header (and any other occurrences in that component) from waveEnv to blockEnv so the type useWaveEnv<BlockEnv>() stays the same but naming is consistent across components (update all references within BlockFrame_Header to the new identifier).frontend/preview/mock/mockwaveenv.ts (1)
216-228: Mock omits validation present in real handler—acceptable for testing but worth documenting.The real
SetBaseConfigValue(seepkg/wconfig/settingsconfig.go:781-817) validates config keys againstSettingsTypefields and checks value types before merging. This mock implementation intentionally skips those validations, which is fine for unit tests but means tests won't catch invalid config keys or type mismatches.Consider adding a brief inline comment noting this difference for future maintainers:
📝 Suggested documentation
+ // Note: Unlike the real SetBaseConfigValue, this mock skips key/type validation. + // Tests using this mock won't catch invalid config keys or type mismatches. dispatchMap.set("setconfig", async (_client, data: SettingsType) => { const current = globalStore.get(wos.fullConfigAtom);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/mockwaveenv.ts` around lines 216 - 228, The mock handler registered with dispatchMap.set("setconfig") merges provided data into globalStore.get(wos.fullConfigAtom) without validating keys or types against SettingsType; update the mock by adding a concise inline comment above the handler that explicitly states this differs from the real SetBaseConfigValue (pkg/wconfig/settingsconfig.go:781-817) which validates config keys and types, and that the mock intentionally skips validation for testing purposes so maintainers know tests won't catch invalid keys/type mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/block/blockframe-header.tsx`:
- Line 179: The file has inconsistent variable naming: BlockFrame_Header uses
waveEnv from useWaveEnv<BlockEnv>() while HeaderEndIcons and other places use
blockEnv; rename the variable in BlockFrame_Header (and any other occurrences in
that component) from waveEnv to blockEnv so the type useWaveEnv<BlockEnv>()
stays the same but naming is consistent across components (update all references
within BlockFrame_Header to the new identifier).
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 216-228: The mock handler registered with
dispatchMap.set("setconfig") merges provided data into
globalStore.get(wos.fullConfigAtom) without validating keys or types against
SettingsType; update the mock by adding a concise inline comment above the
handler that explicitly states this differs from the real SetBaseConfigValue
(pkg/wconfig/settingsconfig.go:781-817) which validates config keys and types,
and that the mock intentionally skips validation for testing purposes so
maintainers know tests won't catch invalid keys/type mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 787df850-5d9a-4460-8d07-a81fad2bdd67
📒 Files selected for processing (3)
frontend/app/block/blockenv.tsfrontend/app/block/blockframe-header.tsxfrontend/preview/mock/mockwaveenv.ts
1 participant
No description provided.